-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf(jest-runtime): load chalk
only once per worker
#10864
Conversation
See also comments like this in one of our perf-related issues. |
And I know I know, changelog :D |
I think in the future, benchmark test should be introduced for |
Would it make sense to move logging to a dedicated worker, eventually? |
7f25c06
to
49a5ac9
Compare
Seems like we'll need to drop Node 10 for this then, because of |
Other than that it should be good to go as is now, wdyt @SimenB |
RIP Windows (and possibly Mac?) users when checking out these 500 new files 😂 |
Node 10 has https://nodejs.org/docs/latest-v10.x/api/modules.html#modules_module_createrequirefrompath_filename, you can check which exists before using |
Can we generate the files in the test and not check them in? Regardless of diff size, it just seems overkill 😛 |
@jeysal Wouldn't
The polyfill is also really simple if you need to go lower than that https://github.com/nuxt-contrib/create-require/blob/420682d526a8cbe08df4d2796f4be627d02b1ffd/create-require.js#L30-L37 |
Meh, knew about that one but thought that was a shit solution - let's see when 27 actually happens, if it's close to 30 Apr we might as well release on 1 May and stick the 'drop Node 10' change in there as well so we don't have Node 10 around for another year until the next major 😅
I'd say likely yes. It'll definitely always resolve the path properly, and I'd expect given an absolute
I'll add a script that generates them and gitignore them. Although TBH this thing will be barely worth more than a readme at this point. The benchmark already doesn't include a runner script (because others like |
const Module = require('module')
const createRequire = Module.createRequire || Module.createRequireFromPath; Doesn't seem too 💩 I deffo want 27 to support node 10, lots of stuff in this release it'd be nice to make available to people stuck on slightly older versions |
Yeah it's not much code, was more about introducing code that we don't need anymore in three months 😅 |
Anyway, pushed the createRequire change |
Yeah so I ran a benchmark for 26 vs 27 with #7792 and this on the Babel repo for a more realistic example, and as expected the impact of this on most real projects with real tests (and usually not just one small one per file) will be quite small, on Babel e.g. it's 1.04 ± 0.00 times faster. At least I got consistent test run times though (that's why it's ± 0.00), so it's not just random deviation, but measurable. |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
chalk
is imported so many times, in so manyutil
or whatever dependencies, that it's pratically impossible to trace and maintain which imports need to use the 'require outside' option. This is a simple alternative for such cases. Feels a little sketchy adding such a hardcoded list tojest-runtime
, but since it's just for perf and can theoretically be removed at any time without changing behavior, I think it's fine.Test plan
What it does is tested, but I'm thinking I should probably add some perf tests? At least in the 'manual form' that we have e.g. in
jest-worker
, so that my results are reproducible.While #7792 gave ~50% per-test file speedup on Windows, this will give another ~15% on top, bringing it to a total of ~70% per-test file speedup over
26.6.3
.